Open
Conversation
### Desc - 테스트 케어콜 `/care-call/test` 개선 - 개발자들이 통화 아웃바운드 기능 테스트용으로 활용하며, 정식 프로덕션 기능이 아니다 -> 데이터 저장이 불필요 - 더미 Elder, `TEST_SETTING_ID = -1`인 더미 CareCallSetting을 활용 - 인바운드에서 settingId < 0이면 케어콜 데이터 저장 및 분석 이벤트 발행을 건너뛰도록 처리 - 즉시 케어콜 `/care-call/immediate` 개선 - cron으로 예약된 시간에 케어콜이 발송되도록 서비스가 제공되고 있어, 베타테스트의 편의를 위해 즉시 케어콜을 수신할 수 있도록 기능으로 제공한다 - 실제 DB의 Elder, CareCallSetting를 조회하고, id값을 사용하여 비즈니스 로직과 동일한 경로를 타도록 처리 - 통화 결과가 웹훅으로 돌아오면 테스트 케어콜과는 달리, 실제 settingId이므로 DB 저장 및 분석 이벤트도 정상 실행 - 관련 javadoc 명세 추가 및 보강
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Walkthrough테스트 케이스를 구분하기 위해 음수 settingId를 사용하는 로직이 추가되었습니다. 컨트롤러 엔드포인트에 Javadoc이 추가되었고, CareCallSettingService의 의존성이 리포지토리로 변경되었으며, 테스트용 설정 생성 메서드가 제거되었습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/medicare_call/controller/CareCallController.java (1)
70-87:⚠️ Potential issue | 🟡 Minor
/care-call/immediate는 인증이 필요하지만 사용자 정보를 추출하지 않고 있어요
SecurityConfig에서/care-call/test는 의도적으로permitAll에 포함되어 있고 ("//제거 예정" 주석),/care-call/immediate는.anyRequest().authenticated()에 해당되어 JWT 토큰이 필수입니다.다만
/care-call/immediate는 인증 자체는 필요하지만@AuthUser가 없어서 어떤 사용자가 요청을 보냈는지 추출하지 않습니다.upsertCareCallSetting,getCareCallSetting은@AuthUser로 사용자를 검증하는데,/care-call/immediate는 실제 DB 데이터를 건드리면서 요청자를 추적하지 않고 있습니다. 일관성을 위해@AuthUser파라미터 추가를 검토해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/example/medicare_call/controller/CareCallController.java` around lines 70 - 87, The sendImmediateCareCall method in CareCallController requires authentication but does not extract the caller; add an `@AuthUser-annotated` parameter (e.g., AuthUser authUser) to sendImmediateCareCall, use authUser to obtain the requesting user's id (or principal) and pass that identity into careCallTestService.sendImmediateCall (or adjust the service signature) so the call is attributed to the authenticated user, mirroring how upsertCareCallSetting and getCareCallSetting validate/track users.
🧹 Nitpick comments (2)
src/main/java/com/example/medicare_call/service/carecall/outbound/CareCallTestService.java (1)
64-76: 더미 Elder의phone필드 미사용Line 68에서
phone("01011111111")을 builder에 세팅하지만, 실제requestCall(Line 75)에서는req.phoneNumber()를 사용합니다. 더미 elder의 전화번호 필드는 아무데도 쓰이지 않으니 제거하면 코드가 더 명확해집니다.♻️ 정리 제안
Elder testElder = Elder.builder() .id(100) .name("김옥자") - .phone("01011111111") .gender((byte)0) .relationship(ElderRelation.CHILD) .residenceType(ResidenceType.ALONE) .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/example/medicare_call/service/carecall/outbound/CareCallTestService.java` around lines 64 - 76, The dummy Elder created in CareCallTestService.sendTestCall sets phone("01011111111") but that value is never used because careCallClient.requestCall is passed req.phoneNumber(); remove the unused phone(...) call from the Elder.builder() invocation (or alternatively, if you intended to use the dummy number, pass testElder.getPhone() into requestCall) so that either the builder or the requestCall consistently uses the same source; update the Elder builder invocation to omit phone(...) and any related unused getters to keep the test code clear.src/test/java/com/example/medicare_call/service/carecall/outbound/CareCallTestServiceTest.java (1)
44-75:sendImmediateCall— 케어콜 설정 미존재 케이스 테스트 빠짐
sendImmediateCall_elderNotFound는 있는데, 어르신은 있지만CareCallSetting이 없는 경우 (CARE_CALL_SETTING_NOT_FOUND)는 테스트가 없네요. 서비스 코드에서 명시적으로 처리하고 있으니 커버리지 차원에서 추가해 주면 좋겠습니다.✅ 추가 테스트 케이스 예시
`@Test` `@DisplayName`("즉시 케어콜 발송 실패 - 케어콜 설정 없음") void sendImmediateCall_settingNotFound() { // given Long elderId = 1L; Elder elder = Elder.builder().id(1).name("홍길동").build(); when(elderRepository.findById(1)).thenReturn(Optional.of(elder)); when(careCallSettingRepository.findByElder(elder)).thenReturn(Optional.empty()); // when & then assertThatThrownBy(() -> careCallTestService.sendImmediateCall(elderId, CareCallOption.FIRST)) .isInstanceOf(CustomException.class) .extracting("errorCode") .isEqualTo(ErrorCode.CARE_CALL_SETTING_NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/example/medicare_call/service/carecall/outbound/CareCallTestServiceTest.java` around lines 44 - 75, Add a new unit test in CareCallTestServiceTest to cover the missing "care call setting not found" branch: create a test (e.g., sendImmediateCall_settingNotFound) that stubs elderRepository.findById(...) to return an Elder and stubs careCallSettingRepository.findByElder(elder) to return Optional.empty(), then call careCallTestService.sendImmediateCall(elderId, CareCallOption.FIRST) and assert it throws CustomException with errorCode ErrorCode.CARE_CALL_SETTING_NOT_FOUND; also verify careCallRequestSenderService.sendCall is not invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/example/medicare_call/controller/CareCallController.java`:
- Around line 54-57: Update the inaccurate Javadoc on the method handling the
/call-data endpoint in CareCallController: state that a negative settingId
specifically indicates the test endpoint (/care-call/test) which uses
TEST_SETTING_ID = -1 and therefore skips persistence, whereas
/care-call/immediate uses the actual DB settingId and should persist the raw
call data; change the comment text in the Javadoc accordingly so it references
CareCallController and the TEST_SETTING_ID semantics.
In
`@src/main/java/com/example/medicare_call/service/carecall/inbound/CareCallService.java`:
- Around line 41-44: CareCallService.saveCallData can return null for test sends
(settingId < 0), so update CareCallUploadService where you call
CareCallService.saveCallData(processRequest) to null-check the returned
CareCallRecord (saved) before calling saved.getId(); if saved is null, log a
clear message (e.g., "테스트 발송 - 저장 건너뜀") and return or handle appropriately
instead of calling saved.getId(); ensure this avoids NullPointerException and
preserves current control flow expectations.
In
`@src/main/java/com/example/medicare_call/service/carecall/outbound/CareCallTestService.java`:
- Around line 43-44: In sendImmediateCall in CareCallTestService you silently
convert Long elderId to int via elderId.intValue() which can overflow; update
the code to avoid implicit narrowing by either (A) changing the Elder ID domain
to Long everywhere and use elderRepository.findById(elderId) (preferred), or (B)
if repository truly requires int, explicitly check elderId against
Integer.MIN_VALUE/MAX_VALUE and throw a clear IllegalArgumentException before
calling elderRepository.findById(elderId.intValue()); locate usages of
sendImmediateCall and the elderRepository.findById call to ensure consistent ID
types across Elder, repository, and method signatures.
---
Outside diff comments:
In `@src/main/java/com/example/medicare_call/controller/CareCallController.java`:
- Around line 70-87: The sendImmediateCareCall method in CareCallController
requires authentication but does not extract the caller; add an
`@AuthUser-annotated` parameter (e.g., AuthUser authUser) to
sendImmediateCareCall, use authUser to obtain the requesting user's id (or
principal) and pass that identity into careCallTestService.sendImmediateCall (or
adjust the service signature) so the call is attributed to the authenticated
user, mirroring how upsertCareCallSetting and getCareCallSetting validate/track
users.
---
Nitpick comments:
In
`@src/main/java/com/example/medicare_call/service/carecall/outbound/CareCallTestService.java`:
- Around line 64-76: The dummy Elder created in CareCallTestService.sendTestCall
sets phone("01011111111") but that value is never used because
careCallClient.requestCall is passed req.phoneNumber(); remove the unused
phone(...) call from the Elder.builder() invocation (or alternatively, if you
intended to use the dummy number, pass testElder.getPhone() into requestCall) so
that either the builder or the requestCall consistently uses the same source;
update the Elder builder invocation to omit phone(...) and any related unused
getters to keep the test code clear.
In
`@src/test/java/com/example/medicare_call/service/carecall/outbound/CareCallTestServiceTest.java`:
- Around line 44-75: Add a new unit test in CareCallTestServiceTest to cover the
missing "care call setting not found" branch: create a test (e.g.,
sendImmediateCall_settingNotFound) that stubs elderRepository.findById(...) to
return an Elder and stubs careCallSettingRepository.findByElder(elder) to return
Optional.empty(), then call careCallTestService.sendImmediateCall(elderId,
CareCallOption.FIRST) and assert it throws CustomException with errorCode
ErrorCode.CARE_CALL_SETTING_NOT_FOUND; also verify
careCallRequestSenderService.sendCall is not invoked.
src/main/java/com/example/medicare_call/controller/CareCallController.java
Show resolved
Hide resolved
src/main/java/com/example/medicare_call/service/carecall/inbound/CareCallService.java
Show resolved
Hide resolved
Comment on lines
43
to
44
| public String sendImmediateCall(Long elderId, CareCallOption careCallOption) { | ||
| Elder elder = elderRepository.findById(elderId.intValue()) |
There was a problem hiding this comment.
Long → int 묵시적 변환 — 잠재적 오버플로우
elderId.intValue()는 Long 값이 Integer.MAX_VALUE를 초과할 경우 조용히 잘립니다. 현재 Elder ID가 int 타입으로 관리되고 있어 실제로 문제가 되지 않을 수 있지만, 파라미터 타입이 Long인데 내부에서 int로 강제 변환하는 구조 자체가 다소 불일치하네요. 가능하다면 Elder의 ID 타입을 Long으로 통일하거나, 범위 초과 시 명시적인 예외를 던지는 게 더 안전합니다.
🛡️ 방어적 처리 예시
- Elder elder = elderRepository.findById(elderId.intValue())
+ if (elderId > Integer.MAX_VALUE) {
+ throw new CustomException(ErrorCode.ELDER_NOT_FOUND, "유효하지 않은 어르신 ID: " + elderId);
+ }
+ Elder elder = elderRepository.findById(elderId.intValue())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public String sendImmediateCall(Long elderId, CareCallOption careCallOption) { | |
| Elder elder = elderRepository.findById(elderId.intValue()) | |
| public String sendImmediateCall(Long elderId, CareCallOption careCallOption) { | |
| if (elderId > Integer.MAX_VALUE) { | |
| throw new CustomException(ErrorCode.ELDER_NOT_FOUND, "유효하지 않은 어르신 ID: " + elderId); | |
| } | |
| Elder elder = elderRepository.findById(elderId.intValue()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/example/medicare_call/service/carecall/outbound/CareCallTestService.java`
around lines 43 - 44, In sendImmediateCall in CareCallTestService you silently
convert Long elderId to int via elderId.intValue() which can overflow; update
the code to avoid implicit narrowing by either (A) changing the Elder ID domain
to Long everywhere and use elderRepository.findById(elderId) (preferred), or (B)
if repository truly requires int, explicitly check elderId against
Integer.MIN_VALUE/MAX_VALUE and throw a clear IllegalArgumentException before
calling elderRepository.findById(elderId.intValue()); locate usages of
sendImmediateCall and the elderRepository.findById call to ensure consistent ID
types across Elder, repository, and method signatures.
### Desc - 주석 설명이 잘못되어 있는 부분 변경
### Desc - 인바운드 파이프라인에서 분기 처리가 올바르게 되는지 검증
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Desc
/care-call/test개선TEST_SETTING_ID = -1인 더미 CareCallSetting을 활용/care-call/immediate개선Summary by CodeRabbit
릴리스 노트
Documentation
Refactor
Tests